Fix missing authorization check before REST project remix creation#864
Conversation
abcampo-iry
changed the title
Test coverage91.46% line coverage reported by SimpleCov. |
There was a problem hiding this comment.
Pull request overview
Adds an explicit CanCanCan authorization gate to the REST project remix creation endpoint to ensure callers can view the original project before a remix is created, closing an authorization bypass described in digital-editor-issues#1468 and aligning REST behavior with existing GraphQL/Scratch remix flows.
Changes:
- Enforced
authorize! :show, projectbefore invokingProject::CreateRemixin the REST remixes controller. - Added request specs covering unauthorized remix attempts (other user’s project; student vs teacher-only lesson project), asserting
403 Forbidden, no remix created, and no clone operation invoked.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/controllers/api/projects/remixes_controller.rb | Adds an explicit :show authorization check before remix creation to prevent unauthorized cloning. |
| spec/requests/projects/remix_spec.rb | Adds regression coverage for forbidden remix creation scenarios and asserts Project::CreateRemix is not called. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| context 'when the original project belongs to another user' do | ||
| let!(:original_project) { create(:project, user_id: create(:user).id) } | ||
|
|
||
| it 'returns forbidden without creating a remix' do | ||
| allow(Project::CreateRemix).to receive(:call).and_call_original | ||
|
|
||
| expect do | ||
| post("/api/projects/#{original_project.identifier}/remix", params: { project: project_params }, headers:) | ||
| end.not_to change(Project, :count) | ||
|
|
||
| expect(response).to have_http_status(:forbidden) | ||
| expect(Project::CreateRemix).not_to have_received(:call) | ||
| end | ||
| end | ||
|
|
||
| context 'when a student cannot view the teacher-only original project' do | ||
| let(:student) { create(:student, school:) } | ||
| let(:teacher) { create(:teacher, school:) } | ||
| let(:school_class) { create(:school_class, school:, teacher_ids: [teacher.id]) } | ||
| let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'teachers') } | ||
| let!(:original_project) do | ||
| lesson.project.tap do |project| | ||
| project.update!(school:, user_id: teacher.id, instructions: 'Teacher-only instructions') | ||
| end | ||
| end | ||
|
|
||
| before do | ||
| create(:class_student, school_class:, student_id: student.id) | ||
| authenticated_in_hydra_as(student) | ||
| end | ||
|
|
||
| it 'returns forbidden without creating a remix' do | ||
| allow(Project::CreateRemix).to receive(:call).and_call_original | ||
|
|
||
| expect do | ||
| post("/api/projects/#{original_project.identifier}/remix", params: { project: project_params }, headers:) | ||
| end.not_to change(Project, :count) | ||
|
|
||
| expect(response).to have_http_status(:forbidden) | ||
| expect(Project::CreateRemix).not_to have_received(:call) | ||
| end | ||
| end |
There was a problem hiding this comment.
I've been thinking about the testing around this (not just for the PR but for request/feature specs).
A lot of the detail in this test is actually testing the behaviour of the ability rather than the controller and it requires a lot of setup to do that. I agree that there should be at least one test at this level that is checking a 'forbidden' state but maybe any more cases should be pushed down to the ability.
What I think would be more valuable, is configuring our controllers so we we can't forget to check an ability for an action.
There was a problem hiding this comment.
I saw in the ticket.
Agreed.
I can remove the test from there and then trying to go down the road that you described here:
As part of this we might want to consider adding check_authorization to our application controller. This is a cancancan feature that prevents us from 'forgetting' to add in authorization to a controller and would require us to explicitly decide if a controller shouldn't authorize resources (you can do a similar thing with authorize user, although that's not the problem here.)
https://github.com/CanCanCommunity/cancancan/blob/develop/docs/changing_defaults.md#check_authorization
I can create a ticket and add a different PR
There was a problem hiding this comment.
I can create a ticket and add a different PR
Thanks. Might be one for next sprint.
zetter-rpf
left a comment
zetter-rpf
left a comment
There was a problem hiding this comment.
Nice one on fixing this, I've added some thoughts I've had about how we setup and test abilities (up to you if you think it's worth changing as part of this).
Status
Why
The REST project remix endpoint authenticated the caller but did not verify that the caller could view the original project before cloning it. That meant a user who knew a project
identifier could create a remix from a project they were not authorized to read.
This now aligns the REST remix flow with the existing Scratch and GraphQL remix authorization behavior.
What Changed
Follow-up: consider enabling CanCanCan check_authorization at the API controller layer so missing authorization becomes fail-closed by default. This should be handled separately because it requires auditing existing controllers and adding explicit skip_authorization_check for intentionally public or non-CanCan endpoints.